Conversation
tbu-
left a comment
tbu-
left a comment
There was a problem hiding this comment.
I think the API additions look sane to me, even if they're not official String API.
| Err(_) => return Err(CapacityError::new(())), | ||
| } | ||
| } | ||
| self.set_len(len); |
There was a problem hiding this comment.
Why is this implemented as unsafe instead of using ArrayString::try_push?
There was a problem hiding this comment.
Because I think that this function should be as performant as possible, and calling try_push will modify the len on every iteration. This way we can set it once when we're done, which seems to be more efficient.
There was a problem hiding this comment.
Could you write a quick benchmark verifying that this improves performance over the naive try_push loop?
There was a problem hiding this comment.
Sure, here are results on my machune:
test try_extend_bench ... bench: 28 ns/iter (+/- 2) = 146285 MB/s
test try_push_loop ... bench: 336 ns/iter (+/- 13) = 12190 MB/s
I also fixed the other benches (at they were all optimized on my machine in just one LOAD of a capacity constant)
|
Ping? |
2 participants
This is the best effort implementation that allows pushing iterator into
ArrayString. There is noTryFromIteratortrait yet andTryFrom<I>breaks blanketTryFrom<T>impl in std, so this is the next best thing - just a couple of inherent struct functions.Related #142